Skip to content

docs: #1103 round 2 whole-repo prose sweep - #2415

Closed
JarryShaw wants to merge 1 commit into
mainfrom
sweep/1103-round2-whole-repo-prose
Closed

JarryShaw wants to merge 1 commit into
mainfrom
sweep/1103-round2-whole-repo-prose

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Followed the coding style (make pylint, make mypy, make isort)

  • make test passes, and a test case covers the change

  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A, prose only and not user-visible, as for round 1 (docs: #719-style prose sweep after the #1090-1102 defect wave #2408), which carries no entry either

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

Round 2 of the #1103 repeating #719-style sweep. Round 1 (#2408) was scoped to one recent PR wave; this round covers the ground that wave did not touch — all of pcapkit/ outside const/ and vendor/, docs/source/, the root documents, util/, examples/ and tests/. 43 files, prose only (docstrings, comments, .rst/.md text), no behaviour change, no renames.

1. Cross-references converged on the defining class

Every pcapkit.protocols.protocol.Protocol.<member> target under pcapkit/ now names ProtocolBase. Protocol declares only __init_subclass__; payload, unpack, alias, info, read, pack, analyze, _file, __header__, _decode_next_layer, _import_next_layer and the rest are all defined on ProtocolBase. This finishes the direction round 1 took in the register/register_protocol docs — before it the tree was split roughly evenly, with the same members (alias, unpack, from_data, analyze, expand_comp, _lookup_registry) spelled both ways in different modules.

Three places already read ProtocolBase.X in their display text over a Protocol.X target, which is the mismatch round 1 fixed in link.py and transport.py: utilities/chardet.py, protocols/data/data.py, internet/ipv6_ext.py. pcapkit.protocols.data.protocol.Protocol.packet is a different class and is left alone.

One thing to rule on: docs/source/ and tests/ still spell it the other way — 26 occurrences across 12 .rst pages and 5 test modules. That is not accidental: docs/source/pcapkit/protocols/protocol.rst renders these members under the Protocol entry and its ProtocolBase section links on to them as :attr:~Protocol.name``. So both targets resolve, and finishing the conversion there means deciding where the members should render, which is a structural docs call rather than a sweep one. Flagged rather than taken.

2. Prose corrected against the code

  • schema/internet/hip.py — parameter_padding_len claimed every parameter schema in the module as its caller. LocatorSetParameter uses locator_set_padding_len, which that field's own doc-comment already said.
  • conventions/utilities-or-corekit.rst — omitted timestamp conversion from what pcapkit.utilities holds, which pcapkit/utilities/__init__.py's own docstring lists and pcapkit/utilities/datetime.py provides.
  • CONTRIBUTING.md — said a red linter will not fail a pull request. The Lint job's actionlint step carries no continue-on-error and ends on exit "$status", as lint.yml's own header records ("Every step but actionlint is advisory").
  • protocols/index.rst — the class-hierarchy graph drew HIP single-parented. It is class HIP(IPv6_Ext[...], Internet[...], ...), the only class under protocols/internet/ with a second Internet base, and the graph records AH's and ESP's second parent the same way.

3. Before/after narration → present tense

  • internet/mh.py — two EnumLookup notes narrated a rejected get override ("the override was deleted, not widened to accept default"). The reason it would be redundant is kept; the history is dropped.
  • data/internet/hip.py — cert_group's doc-comment repeated the Diffie-Hellman-registry history that its own .. versionchanged:: 1.5.0 note already carries.
  • tests/project/test_annotation_names.py — no longer dates the trap in months.

4. Counts of codebase items dropped (the ruling on #1103)

  • tests/const/test_const_enum_builtin_parity.py cited "113 of the 117 modules" under pcapkit/const/; the tree holds 124, and neither figure is asserted or computed anywhere in the module. The decision and its reasoning are kept.

Defects found while sweeping, reported rather than fixed

These are code, not prose, and are left for their own issues:

  1. FTP.make's code branch is unguarded (protocols/application/ftp.py). code_val = int(code) has no type guard, where the sibling cmmd branch got one on fix(ftp): make raises a bare AttributeError for a command that is not str or bytes #2290 and args on fix(application): FTP.make's args rejects bytearray/int with AttributeError instead of ProtocolError #2410. Measured on this branch: FTP.make(obj, code='abc') raises a bare ValueError: invalid literal for int() with base 10, not the ProtocolError the Raises: section promises; code=True is accepted and writes b'001' although the cmmd guard rejects a bool explicitly; code=3.7 is accepted and writes b'003'. (Repro needs FTP.make on an uninitialised instance — FTP().make(code=...) takes the else arm and raises ProtocolError: FTP: invalid packet type.)
  2. Deferred.__init__'s proto annotation is too narrow (foundation/reassembly/data/data.py). It is proto: 'TransType' with analyze: 'Callable[[TransType, bytes], ProtocolBase]', but TCP.submit passes (bufid[1], bufid[3]) — a port pair — because Transport.analyze overrides the signature to ports: tuple[int, int]. Behaviour is correct; the annotation and the proto: Payload protocol type. docstring describe only the IP case.
  3. test_actionlint_in_lint_1967.py's stated reason does not justify its floor. The docstring says 1.7.12 is needed because "the workflows use YAML anchors, which 1.7.7 cannot read", but lint.yml's own step comment says anchors need 1.7.10 or later. MINIMUM = (1, 7, 12) is what the test asserts, so the real reason for 1.7.12 is recorded nowhere. Not fixed because inventing a reason would be worse.
  4. test_file_size_caps_1969.py scans one of two callers. WORKFLOW = 'unit-tests.yml', but grep -rln cap_file_size .github/workflows/ returns unit-tests.yml and experimental.yml, so an uncapped pytest step added to experimental's Linux job fails nothing. (The Windows leg's uncapped step is deliberate — resource is unavailable there.)
  5. ext.rst and the conventions pages disagree on HIP's classification. ext.rst's shipped-classes table lists HIP as a plain Internet-layer row beside IPX while MH sits under IPv6 Extension Header, though both derive from IPv6_Ext. Which side of HIP's dual nature that grid should show is a presentation call.

Scope not reached

Honest about the edges, so a round 3 knows where to start: docs/source/contributing/workflows.rst (2055 lines) and releasing.rst (613) got the mechanical passes but no line-by-line read; the per-protocol API pages under docs/source/pcapkit/ were covered by the structural sweeps only; the repetitive per-field #: comments in the largest schema/ and data/ modules were sampled rather than read exhaustively; and the bodies of the largest tests/project/ modules (test_release_gates.py, test_ci_timing.py, test_conventions_doc_claims.py) were swept mechanically rather than read.

Test plan

  • AST-structure guard — ast.dump via util/vendor_code_changed.py's code_of, with docstrings and bare string statements stripped: all 40 touched .py files are prose-only against 27c797d35, 0 differences. The 3 remaining files are CONTRIBUTING.md and two .rst pages.
  • No added line exceeds the 120-column --max-line-length; format/docstyle findings on the touched files are all pre-existing lines this PR does not touch.
  • Docs, contract and project suites — tests/test_docstring_contract.py, tests/test_base_class_contract.py, tests/project/test_conventions_doc_claims.py, test_documentation_claims.py, test_docs_issue_pr_roles_1103_unit.py, test_issue_refs_in_comments.py, test_annotation_names.py, test_docs_glossary_1949_unit.py, test_docs_internal_refs_1940_unit.py, test_docs_internal_refs_1947_unit.py, test_docs_ambiguous_xrefs_1775_unit.py, test_docs_unresolvable_roles_1919_unit.py, test_docs_missing_directives_2234_unit.py, test_docs_warns_xrefs_2094_unit.py, test_pr_template_commit_types_1102_unit.py, test_python_versions_1660.py, test_utilities_import_boundary_1735_unit.py, test_no_moved_name_re_exports_1744_unit.py, tests/const/test_const_enum_builtin_parity.py — 186 passed, 22 skipped, 1604 subtests passed.
  • Library suites over every touched package, at make test's exclusions (--ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py') — tests/protocols, tests/foundation, tests/corekit, tests/utilities, tests/interface: 3332 passed, 13 skipped, 29807 subtests passed, 0 failures.
One thing worth recording, since it looks alarming out of context

Running those same directories without the make test exclusions gives 89 failures here. They are all in the fixture-dependent *_runtime.py tier, which reads generated captures: tests/foundation/traceflow/test_traceflow_snapped_capture_runtime.py's own docstring says "The module reads the generated tcp.pcap, so it belongs to the fixture-dependent tier", and this checkout's examples/captures/ holds only in.pcap and dhcp.pcapng because make samples has not been run. make test excludes that tier by design, make test-all depends on samples, and main's Unit Tests run is green at 27c797d35 — this PR's base. Nothing to do with this change, which the AST guard shows cannot alter behaviour.

Round 2 of the #1103 repeating #719-style sweep. Where round 1 (#2408) was
scoped to one recent PR wave, this round covers the ground that wave did not
touch: all of pcapkit/ outside const/ and vendor/, docs/source/, the root
documents, util/, examples/ and tests/. Prose only -- docstrings, comments and
.rst/.md text -- with no behaviour change and no renames.

Cross-references converged on the class that defines the member:

* every `pcapkit.protocols.protocol.Protocol.<member>` target under pcapkit/
  now names `ProtocolBase`, which is where all of them are defined -- `Protocol`
  declares only `__init_subclass__`. This finishes the direction round 1 took in
  the `register`/`register_protocol` docs; the tree had been split roughly evenly,
  with the same members (`alias`, `unpack`, `from_data`, `analyze`, `expand_comp`,
  `_lookup_registry`) spelled both ways.
* display text that already read `ProtocolBase.X` over a `Protocol.X` target is
  no longer a mismatch (utilities/chardet.py, protocols/data/data.py,
  internet/ipv6_ext.py), and `pcapkit.protocols.data.protocol.Protocol.packet`
  -- a different class -- is left alone.

Prose corrected where it did not match the code:

* schema/internet/hip.py's `parameter_padding_len` named every parameter in the
  module as its caller; `LocatorSetParameter` uses `locator_set_padding_len`,
  which the field's own doc-comment already said.
* conventions/utilities-or-corekit.rst omitted timestamp conversion from what
  pcapkit.utilities holds, which utilities/__init__.py's own docstring lists.
* CONTRIBUTING.md said no red linter fails a pull request; lint.yml's actionlint
  step carries no `continue-on-error` and exits on its status, as that workflow's
  own header records.
* the class-hierarchy graph in protocols/index.rst drew HIP single-parented; it
  derives from both `IPv6_Ext` and `Internet`, the only internet class that does,
  and the graph records AH's and ESP's second parent the same way.

Before/after narration replaced with present tense:

* internet/mh.py's two `EnumLookup` notes narrated a rejected `get` override
  ("deleted, not widened to accept `default`"); the reason it would be redundant
  is kept, the history dropped.
* data/internet/hip.py's `cert_group` repeated the Diffie-Hellman-registry
  history already carried by its `.. versionchanged:: 1.5.0` note.
* tests/project/test_annotation_names.py no longer dates the trap in months.

Counts of codebase items dropped, per the ruling on #1103:

* tests/const/test_const_enum_builtin_parity.py cited 113 of 117 const modules;
  the tree holds 124, and neither figure is asserted anywhere in the module.

AST guard (ast.dump via util/vendor_code_changed.py's `code_of`, docstrings and
bare string statements stripped) confirms all 40 touched .py files are prose-only
against 27c797d; no added line exceeds the 120-column limit.
@github-actions github-actions Bot added this to the PyPCAPKit v1.5 milestone Oct 28, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 28, 2026
@JarryShaw JarryShaw added docs Pull requests that change documentation only (docs: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 91.73% (unit tier, Python 3.15, 16bf212f1, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19609 879 2620 866 92.13%
pcapkit/corekit 2596 74 818 37 96.51%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 3045 24 1062 26 98.78%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17578 193 4670 179 98.32%
pcapkit/toolkit 924 36 334 10 95.55%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 4449 1961 1020 167 52.64%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Review: NEEDS CHANGES at 16bf212f (Sonnet, read-only cross-review).

Core claim independently verified: Protocol is a docstring + __init_subclass__ only, all 36 converted members confirmed to exist on ProtocolBase and not Protocol, zero Protocol.<m> targets remain under pcapkit/. AST guard clean on all 40 files. Test figures reproduced exactly (186/22/1604 and 3332/13/29807); the 89-failure caveat on an unexcluded run is confirmed environmental (identical ids on PR and main).

Two fixes needed:

  1. Four link display-texts still read Protocol.X over a ProtocolBase.X target: pcapkit/protocols/protocol.py:2179, pcapkit/protocols/schema/internet/ipv6_route.py:271 (this PR's own edit only changed the target, not the display text, creating the mismatch it set out to remove), plus two pre-existing ones at pcapkit/protocols/misc/null.py:75 and pcapkit/protocols/application/httpv2.py:237.
  2. PR body undercounts: the docs/tests residue is 31 occurrences (26 docs + 5 tests), not 26; the conversion moves 63 rendered links from full member docs onto docstring-less stubs in the built HTML (Protocol.<m> hrefs go from 154 to 91) — worth stating plainly since it's the actual rendering effect, not just a cross-reference fix.

Posting the docs-structure question (whether protocol.rst should restructure so ProtocolBase hosts the full entries, vs. reversing this PR's direction) separately on #1103 as a decision item — it's a real doc-architecture call, not something to decide inside a review.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 28, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Closing — the owner ruled on #1103 that Protocol should be preferred over ProtocolBase in cross-references (ProtocolBase is the internal hidden API), which is the opposite direction from this PR's main conversion. Will fold the correct-direction standardization into the next #1103 round, and open a separate corrective PR for #2408's already-merged change in the same direction.

@JarryShaw JarryShaw closed this Oct 28, 2026
@JarryShaw JarryShaw removed docs Pull requests that change documentation only (docs: subject prefix) review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 28, 2026
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 28, 2026
@github-actions github-actions Bot removed this from the PyPCAPKit v1.5 milestone Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant